Phase 3: hygiene, dependencies, readme - #55
Conversation
…adme jzlib was a pure-Java zlib port last released in 2011, used for the deflate and inflate on the hottest path in the library. java.util.zip does the same job through the JDK's bundled native zlib. Verified byte-identical archive output on the same inputs before removing the dependency, so this changes nothing about what JMPQ3 writes. ZlibStore is gone. compress() with no recompression built a zlib stream of stored blocks, which is by construction larger than its input, so every sector paid for a full copy and an Adler-32 to produce something the caller always discarded by its own "did it shrink" test. It returns null now and the caller stores raw -- byte for byte the same archive, without the work. Its hand-rolled Adler-32 also carried the same signed-overflow bug found in the sector checksums, unreachable only because the output was thrown away. DefaultListfile.txt is test-only, so it moved to src/test/resources rather than adding a megabyte to the published jar. The readme was claiming sparse and bzip2 were unsupported and that (attributes) could not be generated, both untrue since Phase 2. Rewritten around the 2.0 API with a format and feature matrix, and every snippet in it is now compiled and run by ReadmeExampleTests so the next drift is a build failure. Codebeat badge removed; the service is defunct. Also: DegugHelperTests renamed, DebugHelper.bytesToHex no longer allocates for the whole input to print 500 bytes of it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b8d3cbbf7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
No fixture in the repository contains an ADPCM-compressed file -- the breakdown across all 186 entries is 130 zlib, 38 stored, 14 pkware, 2 stored+zlib, 1 huffman, 1 empty. So the exact case that caused attributes generation to be disabled in 2017 is the one case still untested, and the StormLib-golden CRC test the audit asked for does not exist. The write side cannot produce ADPCM either, so the fixture has to come from StormLib or a real map with sound. Split out as P4-3b rather than left as a claim.
Issue #11 was disabled years ago because the CRC32 of some wav files disagreed with StormLib's. A CRC32 over decoded content can only disagree if the decode disagrees, so this settles it by decoding the same file twice. tools/mpqcodecs.py implements Huffman and ADPCM transcribed from StormLib's huff.cpp and adpcm.cpp -- from the C, not from the Java in this repository, because an implementation that agrees by construction proves nothing. Both now decode wavTest.w3x's ReviveNightElf.wav to the same 144464 bytes and the same MD5. That file is the exact shape the issue thread described: sector 0 is plain zlib because it holds the RIFF header, and the other 34 sectors are 0x41, Huffman over ADPCM mono. The committed golden manifest gains that digest, so GoldenFileTests now checks the wav on every run rather than skipping it as undecodable. IssueSampleTests covers the maps named in issues #46 and #47. They are other people's maps so they are not committed; the tests skip unless pointed at a directory with -PissueSamples. Forest Defense from #46 does open now, with its declared header size of 2097410 repaired, and its files extract by name. Coverage reporting is now continue-on-error. fail-on-error only covers the upload, so a 504 from coveralls.io while the action fetched its own reporter failed a job whose build, tests, reference checks and manifest check had all passed. A reporting outage is not a broken build.
The writing example referenced Files, Path and StandardCharsets without importing any of them, so it would not compile for the first person to copy it. ReadmeExampleTests did not catch that, and could not: it runs the same code with the imports on the test class, so it keeps the readme honest about behaviour and says nothing about whether the text is copy-pasteable. ReadmeSnippetCompilationTests compiles the text. Any block carrying at least one import presents itself as complete and has to compile with only the imports it declares; blocks with none are fragments that reference variables the prose introduces, and are left alone. A floor on the number of complete blocks found means the extraction cannot silently stop matching and start verifying nothing. Verified by reverting the readme to the flagged state: the test then names Files, Path and StandardCharsets as missing. The rebuild example gained imports and declarations for the same reason.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b1dc7bebf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The javadoc said -Djmpq3.issueSamples while build.gradle read the project property issueSamples, so following the documented command left the tests skipping while looking like they had run -- the worst failure mode for an opt-in test. The command is now -PissueSamples, with a note on why, and the build forwards the -D form as well since that is the reflex.
Drops
jzlibforjava.util.zip— verified byte-identical archive output before removing the dependency. RemovesZlibStore, whose only caller always discarded its output (and which carried the same signed-overflow Adler-32 bug as P2-3, unreachable for that reason).DefaultListfile.txtis test-only, so it no longer ships in the jar.Readme rewritten for the 2.0 API — it was still claiming sparse/bzip2 were unsupported and that
(attributes)could not be generated. Every snippet is compiled and run byReadmeExampleTests. Codebeat badge removed.Phase 3 and Phase 5 status recorded in
AUDIT.md.